Skip to content

Re-using specs from ad2-getting-started #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 6, 2023
Merged

Re-using specs from ad2-getting-started #8

merged 3 commits into from
Apr 6, 2023

Conversation

bpurinton
Copy link
Contributor

@bpurinton bpurinton commented Apr 5, 2023

This first helper methods project is just a re-factoring, so all of the tests pass and the goal is just to not "break" anything and have the tests keep passing in the end.

A better way might be actually testing for the presence of the helper methods in the source code, rather than just testing the HTML output. But I'm not sure how/if we want to do that.

Things to potentially test for here (in the future off separate PRs):

  • presence of link_to
  • presence of form_with in new.html.erb and edit.html.erb
  • presence of find in controller
  • removal of query_ from form params and change from strings to symbols
  • presence of bundled sub-hashes in forms and mass assignment in controllers
  • presence of resources in routes.rb

Copy link
Contributor

@jelaniwoods jelaniwoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a glance these look good to me!

A better way might be actually testing for the presence of the helper methods in the source code, rather than just testing the HTML output. But I'm not sure how/if we want to do that.

TIL you can write view specs. You might have access to the pre-rendered template so that might be the ideal way to test if someone used a helper method.

Comment on lines 34 to 37
it "has a hidden authenticity token input", points: 2 do
expect(page).to have_selector("input[name='authenticity_token']", visible: false),
"Expected the new movie form to have an input field of type='hidden' and name='authenticity_token'."
end
Copy link
Contributor

@jelaniwoods jelaniwoods Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this again using the "solutions" from helper-methods-3, I don't think this tag is visible from the form_with method.

This always seems to fail for me, even when enabling JavaScript in the test (in case the input is injected later on)

Did this pass for you when the form used form_with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jelaniwoods See my comment about this on the other PR. I know it's weird, but it's actually kind of a good thing that this test doesn't pass with form_with here. We want them to write it out by hand and in the next project helper-methods-3 they get to use form_with and we drop the authenticity token test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpurinton I thought the ending point of helper-methods is the starting point for helper-methods-3, which uses form_with. The current README for this project says to use form_with. Is the idea for this assignment that they also include the authenticity_token helper even though it shouldn't be required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jelaniwoods Ah, you're right. The video recording for this project starts with the form_with refactoring. Removing this spec here then.

@bpurinton bpurinton marked this pull request as draft April 5, 2023 20:29
@bpurinton
Copy link
Contributor Author

@jelaniwoods

TIL you can write view specs. You might have access to the pre-rendered template so that might be the ideal way to test if someone used a helper method.

I experimented with this but the render method still outputs the HTML (here's that branch), so the helper methods are no longer visible to the test.

Can't figure this out! Maybe better to just merge these basic specs and turn my list of additional specs above into an issue, and then eventually separate PRs.

@bpurinton bpurinton marked this pull request as ready for review April 5, 2023 22:33
@bpurinton bpurinton requested a review from jelaniwoods April 5, 2023 22:33
@jelaniwoods
Copy link
Contributor

Can't figure this out! Maybe better to just merge these basic specs and turn my list of additional specs above into an issue, and then eventually separate PRs.

@bpurinton If there isn't a built in helper, you could also open and read the template directly, since we're assuming that their view templates are named conventionally.

Something like:

require "rails_helper"

describe "movies/index" do
  it "uses an embedded Ruby link_to helper method", points: 2 do
    template_name = "movies/index.html.erb"
    template_path = Rails.root.join('app', 'views', template_name)
    template_contents = open(template_path).read
    expect(template_contents).to match /<%= link_to "Show details", movie %>/
  end
end

It's unconventional, but functional.

Copy link
Contributor

@jelaniwoods jelaniwoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current changes look good to me. I previously left a comment about one way to test helper methods in view templates, but that work can be done in a separate PR.

@bpurinton
Copy link
Contributor Author

bpurinton commented Apr 6, 2023

@jelaniwoods I do like your clever test strategy for helper methods, but let me put that idea on a separate PR with these "advanced" or "unconventional" tests and I can see about working on those later / after discussing with Raghu.

Merging these ones for now!

@bpurinton bpurinton merged commit 5dd4e39 into main Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants